Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make javax.servlet 2.5 a provided dependency #89

Merged
merged 1 commit into from
Sep 5, 2013

Conversation

jalpedersen
Copy link
Contributor

When running in a servlet 3.0 environment (jetty 8+) we run into the following security exception as the signer for javax.servlet 2.5 and javax.servlet 3.0 differ (at least in the case of Jetty):

Exception in thread "main" java.lang.SecurityException: class "javax.servlet.AsyncContext"'s signer information   does not match signer information of other classes in the same package

By making the dependency provided we use the underlying container's or adapter's version, which is what we want (of course as long javax.servlet remains backwards compatible with 2.5)

@weavejester
Copy link
Member

Could you ensure the tests pass?

@jalpedersen
Copy link
Contributor Author

Yes, sorry about that. I forgot to run the tests with the dev profile

@weavejester
Copy link
Member

Could you tidy up the commit message? The first line of a git commit message is considered to be special, like a subject line. It should be a brief description of the commit, ideally under 70 characters. So perhaps:

Make javax.servlet a provided dependency
This is so we can run in a servlet 3.0 environment as well (Jetty 8+)

Other than that, the commit looks fine.

This is so we can run in a servlet 3.0 environment as well (Jetty 8+)
@jalpedersen
Copy link
Contributor Author

Commit message has been replaced with your suggestion.

@kul
Copy link

kul commented Dec 10, 2013

@weavejester Do you think instead of making dependency provided, the user can exclude it from the mvn, or lein's project.clj for special cases and include again. This affects compojure and other frameworks which are using ring.

@weavejester
Copy link
Member

Yes, in retrospect this patch may be causing more problems than it solved. On the other hand, it feels like a more "correct" solution not to tie ring-core to a specific servlet-api version. I'm also hesitant to revert a change made in a patch that addresses a legitimate concern.

When I have time to do so, I plan on rewriting the multipart-params middleware to not have a dependency on FileUpload, and therefore servlet-api.

@jarohen
Copy link

jarohen commented Jan 17, 2014

FWIW, +1, this is a real pain. I don't think it's a good idea for projects to declare their dependencies in the README instead of project.clj, and requiring anyone who uses a library to check all of its transitive dependencies manually.

@sunng87
Copy link

sunng87 commented Feb 24, 2014

This is a great patch! And it can be even better by removing dependency to servlet API from ring-core totally.

@weavejester , is the multipart-params middleware rewrite in progress or done ? I'm using a non-servlet http server as ring server, but got the ClassNotFoundException issue on servlet classes. It will be great if we can separate ring-core from Servlet API. You know, as the clojure community grows, we might have more non-servlet ring server.

Edit: fix typo and grammar.

@weavejester
Copy link
Member

The multipart-params rewrite is currently a work in progress, however it's high up my todo list, so I don't expect it to take much longer.

@sunng87
Copy link

sunng87 commented May 28, 2014

@weavejester Any updates on the multipart-param rewrite?

@weavejester
Copy link
Member

The multipart-params rewrite has slipped down my todo list, since it's not an essential update, and there are a number of other things competing for my attention. I'll add it to Ring 1.4.0, but that'll likely be after the summer.

@laczoka
Copy link

laczoka commented Dec 15, 2014

@weavejester Hi, any update on the multipart-param rewrite? :)

@weavejester
Copy link
Member

No progress on it since my last comment. It's in a branch, and I still intend for it to be in Ring 1.4.0, but it won't be released this year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants